#88 Feat: 알림 API 연동#102
Conversation
📝 WalkthroughWalkthroughAdds server-driven sidebar notifications with SSE updates and read actions, while refactoring mock-apply question handling, shared modal/CTA components, analysis failure UI, and scroll-gradient behavior. ChangesNotification API integration
Mock-apply UI and analysis flow
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Lnb
participant NotificationApi
participant NotificationStream
Lnb->>NotificationApi: Fetch current notifications
NotificationApi-->>Lnb: Return notification list
Lnb->>NotificationApi: Subscribe to notification stream
NotificationApi->>NotificationStream: Open SSE connection
NotificationStream-->>Lnb: Deliver new notification
Lnb->>NotificationApi: Mark notification as read
NotificationApi-->>Lnb: Complete read request
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (4)
jobdri/src/hooks/useScrollGradient.ts (1)
20-36: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSpreading a dynamic-length
depsarray intouseEffect's dependency list.
[checkScroll, ...deps](Line 36) works today since the only call site passes a fixed single-element array, but React's hooks lint rule expects the dependency array to have the same number of elements on every render; a future call site with a variably-sizeddepsarray would tripreact-hooks/exhaustive-deps.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@jobdri/src/hooks/useScrollGradient.ts` around lines 20 - 36, The useEffect dependency list in useScrollGradient currently spreads the dynamic-length deps array, which can change its element count between renders. Replace this pattern with a stable dependency strategy that preserves updates when deps values change, without varying the number of entries in the hook dependency array.jobdri/src/components/common/lnb/Lnb.tsx (1)
102-171: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove large commented-out legacy implementation.
Lines 102-171 contain two full commented-out
useEffectblocks (oldfetchMyMockApplies-based recent items loader and the old rawfetch/EventSourcenotification loader) superseded by the working code below (Lines 173-205). Dead code adds noise and risk of accidental reactivation.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@jobdri/src/components/common/lnb/Lnb.tsx` around lines 102 - 171, Remove the two obsolete commented-out useEffect blocks containing the old fetchMyMockApplies recent-items loader and raw fetch/EventSource notification loader. Keep the active implementation beginning below them unchanged.jobdri/src/components/common/lnb/LnbNotification.tsx (1)
414-430: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove debug logging before release.
console.log("클릭한 알림 전체 데이터:", notificationItem)and the accompanyingconsole.warndump the full notification payload (title/description/target ids) on every click — leftover debug artifacts (marked with 🚨/⚠️ emoji) that should be cleaned up.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@jobdri/src/components/common/lnb/LnbNotification.tsx` around lines 414 - 430, Remove the debug console.log and console.warn calls from handleNotificationClick, including the full notificationItem payload dumps and emoji-marked messages. Preserve the existing notification read handling and router navigation behavior, including the missing mockApplyId fallback.jobdri/src/hooks/useNotification.ts (1)
3-3: 🗄️ Data Integrity & Integration | 🔵 Trivial | 💤 Low valueRemove the unused
useNotificationshook.
useNotificationsis only exported fromjobdri/src/hooks/useNotification.tsand has no imports/callers. It also still uses unauthenticatedfetch, wrong wrapper shape, and nativeEventSource, while the live notification path inLnb.tsxuses the new API helpers instead.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@jobdri/src/hooks/useNotification.ts` at line 3, Remove the unused useNotifications hook from useNotification.ts, including its export and any hook-specific imports or supporting code that become unnecessary. Do not alter the active notification implementation or the Lnb.tsx live notification path.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@jobdri/src/app/mockApply/`[mockApplyId]/page.tsx:
- Around line 266-268: Update performDelete so that when the deleted targetId
matches the active selectedId, it also selects a remaining question after
filtering. Preserve the current selection when deleting another question, and
clear or safely reset the selection when no questions remain.
In `@jobdri/src/components/common/lnb/index.ts`:
- Around line 1-8: Restore the Lnb barrel export in the common LNB index so the
existing import in app/credit/page.tsx continues to resolve, or update that
import to reference the Lnb module directly; ensure Lnb remains available
through the chosen import path.
In `@jobdri/src/components/common/lnb/Lnb.tsx`:
- Around line 242-257: Update handleReadItem and handleMarkAllRead to set readAt
to the current timestamp whenever notifications are marked read, while
preserving their existing read-state and hasNotification updates. Ensure both
optimistic paths provide the timestamp required by LnbNotificationList’s
seven-day visibility filtering.
- Around line 173-205: Update the initial notification handling in the
useEffect’s loadInitialNotifications flow so its setNotificationItems update
merges fetched items with notifications already added by
subscribeToNotificationStream, preserving concurrently received SSE
notifications instead of replacing the current state. Keep the fetched
notification ordering and existing hasNotification calculation consistent with
the merged result.
In `@jobdri/src/components/common/lnb/LnbNotification.tsx`:
- Around line 128-149: Update the id mapping in mapApiToLnbItem to treat numeric
id 0 as a valid identifier, generating a UUID only when item.id is nullish or
otherwise absent. Preserve string conversion for all provided ids so
markNotificationAsRead uses the server-side identifier.
In `@jobdri/src/components/common/MockApplyTemplate.tsx`:
- Around line 47-48: Update the nextIconType prop defaults in
MockApplyTemplate’s destructuring at both referenced locations so omitted
callers receive the existing SPARKLE icon. Preserve explicitly provided icon
values and the current nextLabel behavior.
In `@jobdri/src/components/common/modal/ModalOverlay.tsx`:
- Around line 10-14: Update the useEffect in ModalOverlay to capture
document.body.style.overflow before setting it to "hidden", then restore that
captured value in the cleanup instead of forcing "unset", preserving any
pre-existing body overflow setting.
In `@jobdri/src/components/mockApply/ResumeAnalysisLoading.tsx`:
- Around line 17-18: Update ResumeAnalysisLoading and its failure-confirmation
handler to invoke the optional onErrorConfirm callback when provided, falling
back to router.replace("/") only when it is absent; otherwise remove the unused
prop and related declarations.
In `@jobdri/src/lib/api/notification.ts`:
- Around line 60-82: Update subscribeToNotificationStream so fetchEventSource
uses the notifications stream endpoint without the accessToken query parameter;
retain the Authorization header from headers and remove the now-unused tokenOnly
extraction.
---
Nitpick comments:
In `@jobdri/src/components/common/lnb/Lnb.tsx`:
- Around line 102-171: Remove the two obsolete commented-out useEffect blocks
containing the old fetchMyMockApplies recent-items loader and raw
fetch/EventSource notification loader. Keep the active implementation beginning
below them unchanged.
In `@jobdri/src/components/common/lnb/LnbNotification.tsx`:
- Around line 414-430: Remove the debug console.log and console.warn calls from
handleNotificationClick, including the full notificationItem payload dumps and
emoji-marked messages. Preserve the existing notification read handling and
router navigation behavior, including the missing mockApplyId fallback.
In `@jobdri/src/hooks/useNotification.ts`:
- Line 3: Remove the unused useNotifications hook from useNotification.ts,
including its export and any hook-specific imports or supporting code that
become unnecessary. Do not alter the active notification implementation or the
Lnb.tsx live notification path.
In `@jobdri/src/hooks/useScrollGradient.ts`:
- Around line 20-36: The useEffect dependency list in useScrollGradient
currently spreads the dynamic-length deps array, which can change its element
count between renders. Replace this pattern with a stable dependency strategy
that preserves updates when deps values change, without varying the number of
entries in the hook dependency array.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 27adaaad-21ee-40b0-9220-4b41f51ccbbb
⛔ Files ignored due to path filters (1)
jobdri/pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (19)
jobdri/package.jsonjobdri/src/app/mockApply/[mockApplyId]/page.tsxjobdri/src/app/mockApply/[mockApplyId]/result/page.tsxjobdri/src/app/mockApply/[mockApplyId]/result/resume-analysis-loading/ResumeAnalysisLoadingPageClient.tsxjobdri/src/app/page.tsxjobdri/src/components/common/MockApplyTemplate.tsxjobdri/src/components/common/lnb/Lnb.tsxjobdri/src/components/common/lnb/LnbDefault.tsxjobdri/src/components/common/lnb/LnbFolded.tsxjobdri/src/components/common/lnb/LnbNotification.tsxjobdri/src/components/common/lnb/LnbShared.tsxjobdri/src/components/common/lnb/index.tsjobdri/src/components/common/modal/ModalOverlay.tsxjobdri/src/components/mockApply/ResumeAnalysisLoading.tsxjobdri/src/components/mockApply/result/ResumeAnalysisDetail.tsxjobdri/src/constants/notificationIconStyles.tsjobdri/src/hooks/useNotification.tsjobdri/src/hooks/useScrollGradient.tsjobdri/src/lib/api/notification.ts
| const performDelete = async (targetId: string) => { | ||
| setQuestions(questions.filter((q) => q.id !== targetId)); | ||
| }; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Select a remaining question after deleting the active one.
Deleting the selected item leaves selectedId pointing to a removed question, so the form disappears and submission becomes disabled until another item is clicked.
Proposed fix
-const performDelete = async (targetId: string) => {
- setQuestions(questions.filter((q) => q.id !== targetId));
+const performDelete = (targetId: string) => {
+ const remainingQuestions = questions.filter((q) => q.id !== targetId);
+ setQuestions(remainingQuestions);
+ if (selectedId === targetId) {
+ setSelectedId(remainingQuestions[0]?.id ?? null);
+ }
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const performDelete = async (targetId: string) => { | |
| setQuestions(questions.filter((q) => q.id !== targetId)); | |
| }; | |
| const performDelete = (targetId: string) => { | |
| const remainingQuestions = questions.filter((q) => q.id !== targetId); | |
| setQuestions(remainingQuestions); | |
| if (selectedId === targetId) { | |
| setSelectedId(remainingQuestions[0]?.id ?? null); | |
| } | |
| }; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@jobdri/src/app/mockApply/`[mockApplyId]/page.tsx around lines 266 - 268,
Update performDelete so that when the deleted targetId matches the active
selectedId, it also selects a remaining question after filtering. Preserve the
current selection when deleting another question, and clear or safely reset the
selection when no questions remain.
| // export { default as Lnb } from "./Lnb"; | ||
| // export { default as LnbDefault } from "./LnbDefault"; | ||
| // export { default as LnbFolded } from "./LnbFolded"; | ||
| // export { | ||
| // defaultNotificationItems, | ||
| // LnbNotificationPanel, | ||
| // type LnbNotificationItem, | ||
| // } from "./LnbNotification"; |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n -C2 'from ["'"'"']`@/components/common/lnb`["'"'"']' -g '*.ts' -g '*.tsx'Repository: JobDri-Developer/FrontEnd
Length of output: 163
🏁 Script executed:
#!/bin/bash
set -e
echo "== matching imports involving common/lnb =="
rg -n -C2 '["'"'"'][^\s"'"'"'<=-]*/lnb|["'"'"'][^\s"'"'"'<=-]*/components/common/lnb|["'"'"'][^\s"'"'"'<=-]*/lnb/' -g '*.ts' -g '*.tsx' . || true
echo
echo "== matching import from @ path containing lnb =="
rg -n -C2 '["'"'"']@.*lnb|["'"'"']`@/`.*/lnb[<>/]|["'"'"']`@/components/common/lnb`' -g '*.ts' -g '*.tsx' . || true
echo
echo "== export status =="
cat -n jobdri/src/components/common/lnb/index.ts
echo
echo "== lnb barrel exports (direct/indirect references in changed files if available) =="
git diff --stat || true
git status --short || trueRepository: JobDri-Developer/FrontEnd
Length of output: 9896
🏁 Script executed:
#!/bin/bash
set -e
echo "== unresolved barrel imports =="
python3 - <<'PY'
import re, pathlib
for path in pathlib.Path('.').rglob('*.ts'):
text = path.read_text(errors='ignore').splitlines()
for i,line in enumerate(text,1):
m = re.search(r'\bimport\s+(?:type\s+)?(?P<brace_start>(?:{.*?})|\w+|\*\s+as\s+\w+)\s+from\s+[\'"](`@/components/common/lnb`(?:/index)?)[\'"]', line)
print(f"{path}:{i}:{line.strip()}")
PY
echo
echo "== app/credit/page.tsx relevant section =="
cat -n jobdri/src/app/credit/page.tsx | sed -n '1,40p'
echo
echo "== files at exact barrel path =="
find . -path './jobdri/src/components/common/lnb/index.ts' -type f -exec sh -c 'echo "--- $1"; cat -n "$1"' sh {} \;
echo
echo "== git tracked files for Lnb barrel =="
git ls-files | rg '(^|/)jobdri/src/(components|app)/.*Lnb|jobdri/src/components/common/lnb/index\.ts' || trueRepository: JobDri-Developer/FrontEnd
Length of output: 50382
🏁 Script executed:
#!/bin/bash
set -e
echo "== active unresolved barrels =="
rg -n '^\s*[A-Za-z_$][A-Za-z0-9_$,{}<>\s*]*' jobdri/src/app/credit/page.tsx | head -40
echo "--- exact line 12 ---"
sed -n '12p' jobdri/src/app/credit/page.tsx
echo
echo "== unresolved imports to exact barrel with any exported name =="
python3 - <<'PY'
import pathlib, re
names = {'Lnb', 'LnbDefault', 'LnbFolded', 'defaultNotificationItems', 'LnbNotificationPanel', 'LnbNotificationItem'}
for path in pathlib.Path('jobdri/src').rglob('*'):
if path.is_file() and path.suffix in {'.ts', '.tsx'}:
text = path.read_text(errors='ignore').splitlines()
for i,line in enumerate(text,1):
if 'from "`@/components/common/lnb`"' in line or 'from "`@/components/common/lnb/index`"' in line:
brace_start = re.search(r'\{(?P<body>.*?)\}', line)
if brace_start:
if any(n in brace_start.group('body').split() or re.search(r'\b'+re.escape(n)+r'\b', brace_start.group('body')) for n in names):
print(f"{path}:{i}:{line.strip()}")
PYRepository: JobDri-Developer/FrontEnd
Length of output: 2248
Restore the LNB barrel export or update app/credit/page.tsx.
jobdri/src/app/credit/page.tsx:12 still does import { Lnb } from "@/components/common/lnb", but this barrel no longer exports Lnb, so that import will fail. Move it to @/components/common/lnb/Lnb or restore the export.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@jobdri/src/components/common/lnb/index.ts` around lines 1 - 8, Restore the
Lnb barrel export in the common LNB index so the existing import in
app/credit/page.tsx continues to resolve, or update that import to reference the
Lnb module directly; ensure Lnb remains available through the chosen import
path.
| useEffect(() => { | ||
| const loadData = async () => { | ||
| // 1. 기존 알림 먼저 불러오기 | ||
| const loadInitialNotifications = async () => { | ||
| try { | ||
| const data = await fetchMyMockApplies({ | ||
| redirectOnUnauthorized: false, | ||
| }); | ||
| const allItems = [...data.inProgress, ...data.completed]; | ||
|
|
||
| const mappedItems: LnbRecentItem[] = allItems.map((item) => ({ | ||
| id: String(item.mockApplyId), | ||
| companyName: item.companyName, | ||
| jobTitle: | ||
| item.jobTitle || item.detailClassificationName || "직무 미지정", | ||
| version: item.version ?? 1, | ||
| })); | ||
|
|
||
| setRecentItems(mappedItems); | ||
| if (mappedItems.length > 0) { | ||
| setSelectedRecentItemId(mappedItems[0].id); | ||
| const data = await fetchNotifications(); | ||
| if (data.isSuccess && data.result) { | ||
| const mappedItems = data.result.map(mapApiToLnbItem); | ||
| setNotificationItems(mappedItems); | ||
| setHasNotification(mappedItems.some((item) => !item.read)); | ||
| } | ||
| } catch (error) { | ||
| console.error("데이터 로드 실패:", error); | ||
| setRecentItems([]); | ||
| console.error("초기 알림 목록 로드 실패:", error); | ||
| } | ||
| }; | ||
|
|
||
| loadData(); | ||
| loadInitialNotifications(); | ||
|
|
||
| const unsubscribe = subscribeToNotificationStream( | ||
| (newNotification) => { | ||
| const mappedNewItem = mapApiToLnbItem(newNotification); | ||
| setNotificationItems((prev) => [mappedNewItem, ...prev]); | ||
| setHasNotification(true); | ||
| }, | ||
| // 에러가 났을 때 | ||
| (error) => { | ||
| console.error("실시간 알림 연결 문제 발생:", error); | ||
| }, | ||
| ); | ||
|
|
||
| return () => { | ||
| unsubscribe(); | ||
| }; | ||
| }, []); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Initial fetch can clobber a concurrently-arriving SSE notification.
loadInitialNotifications() is fired (not awaited) and then subscribeToNotificationStream is set up immediately after. If the stream delivers a message before the initial fetch resolves, the SSE handler prepends it via setNotificationItems((prev) => [mappedNewItem, ...prev]), but once the fetch resolves, setNotificationItems(mappedItems) performs a full unconditional replace, silently dropping that notification.
🐛 Suggested fix — merge instead of replace
const data = await fetchNotifications();
if (data.isSuccess && data.result) {
const mappedItems = data.result.map(mapApiToLnbItem);
- setNotificationItems(mappedItems);
+ setNotificationItems((prev) => {
+ const existingIds = new Set(mappedItems.map((item) => item.id));
+ const streamOnly = prev.filter((item) => !existingIds.has(item.id));
+ return [...streamOnly, ...mappedItems];
+ });
setHasNotification(mappedItems.some((item) => !item.read));📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| useEffect(() => { | |
| const loadData = async () => { | |
| // 1. 기존 알림 먼저 불러오기 | |
| const loadInitialNotifications = async () => { | |
| try { | |
| const data = await fetchMyMockApplies({ | |
| redirectOnUnauthorized: false, | |
| }); | |
| const allItems = [...data.inProgress, ...data.completed]; | |
| const mappedItems: LnbRecentItem[] = allItems.map((item) => ({ | |
| id: String(item.mockApplyId), | |
| companyName: item.companyName, | |
| jobTitle: | |
| item.jobTitle || item.detailClassificationName || "직무 미지정", | |
| version: item.version ?? 1, | |
| })); | |
| setRecentItems(mappedItems); | |
| if (mappedItems.length > 0) { | |
| setSelectedRecentItemId(mappedItems[0].id); | |
| const data = await fetchNotifications(); | |
| if (data.isSuccess && data.result) { | |
| const mappedItems = data.result.map(mapApiToLnbItem); | |
| setNotificationItems(mappedItems); | |
| setHasNotification(mappedItems.some((item) => !item.read)); | |
| } | |
| } catch (error) { | |
| console.error("데이터 로드 실패:", error); | |
| setRecentItems([]); | |
| console.error("초기 알림 목록 로드 실패:", error); | |
| } | |
| }; | |
| loadData(); | |
| loadInitialNotifications(); | |
| const unsubscribe = subscribeToNotificationStream( | |
| (newNotification) => { | |
| const mappedNewItem = mapApiToLnbItem(newNotification); | |
| setNotificationItems((prev) => [mappedNewItem, ...prev]); | |
| setHasNotification(true); | |
| }, | |
| // 에러가 났을 때 | |
| (error) => { | |
| console.error("실시간 알림 연결 문제 발생:", error); | |
| }, | |
| ); | |
| return () => { | |
| unsubscribe(); | |
| }; | |
| }, []); | |
| useEffect(() => { | |
| // 1. 기존 알림 먼저 불러오기 | |
| const loadInitialNotifications = async () => { | |
| try { | |
| const data = await fetchNotifications(); | |
| if (data.isSuccess && data.result) { | |
| const mappedItems = data.result.map(mapApiToLnbItem); | |
| setNotificationItems((prev) => { | |
| const existingIds = new Set(mappedItems.map((item) => item.id)); | |
| const streamOnly = prev.filter((item) => !existingIds.has(item.id)); | |
| return [...streamOnly, ...mappedItems]; | |
| }); | |
| setHasNotification(mappedItems.some((item) => !item.read)); | |
| } | |
| } catch (error) { | |
| console.error("초기 알림 목록 로드 실패:", error); | |
| } | |
| }; | |
| loadInitialNotifications(); | |
| const unsubscribe = subscribeToNotificationStream( | |
| (newNotification) => { | |
| const mappedNewItem = mapApiToLnbItem(newNotification); | |
| setNotificationItems((prev) => [mappedNewItem, ...prev]); | |
| setHasNotification(true); | |
| }, | |
| // 에러가 났을 때 | |
| (error) => { | |
| console.error("실시간 알림 연결 문제 발생:", error); | |
| }, | |
| ); | |
| return () => { | |
| unsubscribe(); | |
| }; | |
| }, []); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@jobdri/src/components/common/lnb/Lnb.tsx` around lines 173 - 205, Update the
initial notification handling in the useEffect’s loadInitialNotifications flow
so its setNotificationItems update merges fetched items with notifications
already added by subscribeToNotificationStream, preserving concurrently received
SSE notifications instead of replacing the current state. Keep the fetched
notification ordering and existing hasNotification calculation consistent with
the merged result.
| const handleReadItem = (id: string) => { | ||
| setNotificationItems((prev) => { | ||
| const updated = prev.map((item) => | ||
| item.id === id ? { ...item, read: true } : item, | ||
| ); | ||
| setHasNotification(updated.some((item) => !item.read)); | ||
| return updated; | ||
| }); | ||
| }; | ||
|
|
||
| const handleMarkAllRead = () => { | ||
| setNotificationItems((prev) => | ||
| prev.map((item) => ({ ...item, read: true })), | ||
| ); | ||
| setHasNotification(false); | ||
| }; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Optimistic read actions never set readAt, breaking the 7-day auto-hide feature.
handleReadItem and handleMarkAllRead set read: true but leave readAt untouched. LnbNotificationList (in jobdri/src/components/common/lnb/LnbNotification.tsx, Lines 318-331) filters out items only when read && readAt is older than 7 days; if readAt is missing it always keeps the item visible. Since these are the two paths that mark items read locally (markNotificationAsRead/markAllNotificationsAsRead don't return a timestamp, and neither handler sets one), items read through the UI will never be auto-hidden until a full page refetch repopulates readAt from the server.
🐛 Suggested fix — stamp `readAt` optimistically
const handleReadItem = (id: string) => {
setNotificationItems((prev) => {
const updated = prev.map((item) =>
- item.id === id ? { ...item, read: true } : item,
+ item.id === id
+ ? { ...item, read: true, readAt: new Date().toISOString() }
+ : item,
);
setHasNotification(updated.some((item) => !item.read));
return updated;
});
};
const handleMarkAllRead = () => {
setNotificationItems((prev) =>
- prev.map((item) => ({ ...item, read: true })),
+ prev.map((item) => ({
+ ...item,
+ read: true,
+ readAt: item.readAt ?? new Date().toISOString(),
+ })),
);
setHasNotification(false);
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const handleReadItem = (id: string) => { | |
| setNotificationItems((prev) => { | |
| const updated = prev.map((item) => | |
| item.id === id ? { ...item, read: true } : item, | |
| ); | |
| setHasNotification(updated.some((item) => !item.read)); | |
| return updated; | |
| }); | |
| }; | |
| const handleMarkAllRead = () => { | |
| setNotificationItems((prev) => | |
| prev.map((item) => ({ ...item, read: true })), | |
| ); | |
| setHasNotification(false); | |
| }; | |
| const handleReadItem = (id: string) => { | |
| setNotificationItems((prev) => { | |
| const updated = prev.map((item) => | |
| item.id === id | |
| ? { ...item, read: true, readAt: new Date().toISOString() } | |
| : item, | |
| ); | |
| setHasNotification(updated.some((item) => !item.read)); | |
| return updated; | |
| }); | |
| }; | |
| const handleMarkAllRead = () => { | |
| setNotificationItems((prev) => | |
| prev.map((item) => ({ | |
| ...item, | |
| read: true, | |
| readAt: item.readAt ?? new Date().toISOString(), | |
| })), | |
| ); | |
| setHasNotification(false); | |
| }; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@jobdri/src/components/common/lnb/Lnb.tsx` around lines 242 - 257, Update
handleReadItem and handleMarkAllRead to set readAt to the current timestamp
whenever notifications are marked read, while preserving their existing
read-state and hasNotification updates. Ensure both optimistic paths provide the
timestamp required by LnbNotificationList’s seven-day visibility filtering.
| export function mapApiToLnbItem( | ||
| item: ApiNotificationItem, | ||
| ): LnbNotificationItem { | ||
| const mockApplyId = | ||
| item.payload?.mockApplyId !== undefined && | ||
| item.payload?.mockApplyId !== null | ||
| ? String(item.payload.mockApplyId) | ||
| : undefined; | ||
|
|
||
| return { | ||
| id: item.id ? String(item.id) : crypto.randomUUID(), | ||
| title: item.title, | ||
| description: item.body, | ||
| timestamp: formatDate(item.createdAt), | ||
| type: mapNotificationType(item.type), | ||
| read: item.isRead, | ||
| readAt: item.readAt, | ||
| targetType: item.targetType, | ||
| mockApplyId, | ||
| apiType: item.type, | ||
| }; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Falsy-id check mishandles id 0.
item.id ? String(item.id) : crypto.randomUUID() treats a notification id of 0 as absent and substitutes a random UUID, which would then desync from the id used server-side for markNotificationAsRead.
🐛 Suggested fix
- id: item.id ? String(item.id) : crypto.randomUUID(),
+ id: item.id !== undefined && item.id !== null ? String(item.id) : crypto.randomUUID(),📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export function mapApiToLnbItem( | |
| item: ApiNotificationItem, | |
| ): LnbNotificationItem { | |
| const mockApplyId = | |
| item.payload?.mockApplyId !== undefined && | |
| item.payload?.mockApplyId !== null | |
| ? String(item.payload.mockApplyId) | |
| : undefined; | |
| return { | |
| id: item.id ? String(item.id) : crypto.randomUUID(), | |
| title: item.title, | |
| description: item.body, | |
| timestamp: formatDate(item.createdAt), | |
| type: mapNotificationType(item.type), | |
| read: item.isRead, | |
| readAt: item.readAt, | |
| targetType: item.targetType, | |
| mockApplyId, | |
| apiType: item.type, | |
| }; | |
| } | |
| export function mapApiToLnbItem( | |
| item: ApiNotificationItem, | |
| ): LnbNotificationItem { | |
| const mockApplyId = | |
| item.payload?.mockApplyId !== undefined && | |
| item.payload?.mockApplyId !== null | |
| ? String(item.payload.mockApplyId) | |
| : undefined; | |
| return { | |
| id: item.id !== undefined && item.id !== null ? String(item.id) : crypto.randomUUID(), | |
| title: item.title, | |
| description: item.body, | |
| timestamp: formatDate(item.createdAt), | |
| type: mapNotificationType(item.type), | |
| read: item.isRead, | |
| readAt: item.readAt, | |
| targetType: item.targetType, | |
| mockApplyId, | |
| apiType: item.type, | |
| }; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@jobdri/src/components/common/lnb/LnbNotification.tsx` around lines 128 - 149,
Update the id mapping in mapApiToLnbItem to treat numeric id 0 as a valid
identifier, generating a UUID only when item.id is nullish or otherwise absent.
Preserve string conversion for all provided ids so markNotificationAsRead uses
the server-side identifier.
| nextLabel = "다음으로", | ||
| nextIconType, |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Preserve the existing default next icon.
nextIconType is optional, but omitted callers now receive no icon instead of the prior SPARKLE icon. Default this prop to retain the existing CTA contract.
- nextIconType,
+ nextIconType = "SPARKLE",Also applies to: 100-101
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@jobdri/src/components/common/MockApplyTemplate.tsx` around lines 47 - 48,
Update the nextIconType prop defaults in MockApplyTemplate’s destructuring at
both referenced locations so omitted callers receive the existing SPARKLE icon.
Preserve explicitly provided icon values and the current nextLabel behavior.
| useEffect(() => { | ||
| document.body.style.overflow = "hidden"; | ||
| return () => { | ||
| document.body.style.overflow = "unset"; | ||
| }; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Restore the prior body overflow value.
Unmounting currently resets to unset, which can overwrite an existing page-level scroll setting and can re-enable scrolling while another overlay remains mounted.
Proposed fix
useEffect(() => {
+ const previousOverflow = document.body.style.overflow;
document.body.style.overflow = "hidden";
return () => {
- document.body.style.overflow = "unset";
+ document.body.style.overflow = previousOverflow;
};
}, []);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| useEffect(() => { | |
| document.body.style.overflow = "hidden"; | |
| return () => { | |
| document.body.style.overflow = "unset"; | |
| }; | |
| useEffect(() => { | |
| const previousOverflow = document.body.style.overflow; | |
| document.body.style.overflow = "hidden"; | |
| return () => { | |
| document.body.style.overflow = previousOverflow; | |
| }; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@jobdri/src/components/common/modal/ModalOverlay.tsx` around lines 10 - 14,
Update the useEffect in ModalOverlay to capture document.body.style.overflow
before setting it to "hidden", then restore that captured value in the cleanup
instead of forcing "unset", preserving any pre-existing body overflow setting.
| isFailed?: boolean; | ||
| onErrorConfirm?: () => void; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Honor or remove onErrorConfirm.
The new callback prop is never used; failure confirmation always calls router.replace("/"), preventing callers from customizing the recovery action.
Proposed fix
applicationLabel,
isFailed = false,
+ onErrorConfirm,
}: ResumeAnalysisLoadingProps) {- onSecondaryClick={() => router.replace("/")}
+ onSecondaryClick={() => {
+ if (onErrorConfirm) {
+ onErrorConfirm();
+ return;
+ }
+ router.replace("/");
+ }}Also applies to: 32-38, 159-166
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@jobdri/src/components/mockApply/ResumeAnalysisLoading.tsx` around lines 17 -
18, Update ResumeAnalysisLoading and its failure-confirmation handler to invoke
the optional onErrorConfirm callback when provided, falling back to
router.replace("/") only when it is absent; otherwise remove the unused prop and
related declarations.
| export function subscribeToNotificationStream( | ||
| onMessage: (data: ApiNotificationItem) => void, | ||
| onError?: (error: unknown) => void, | ||
| ) { | ||
| const headers = getAuthHeaders(); | ||
| const ctrl = new AbortController(); | ||
|
|
||
| if (!headers.Authorization) { | ||
| console.warn("로그인 토큰이 없어 실시간 알림을 연결하지 않습니다."); | ||
| return () => ctrl.abort(); | ||
| } | ||
|
|
||
| const tokenOnly = headers.Authorization.replace("Bearer ", ""); | ||
|
|
||
| fetchEventSource( | ||
| `${API_BASE_URL}/api/notifications/stream?accessToken=${tokenOnly}`, | ||
| { | ||
| method: "GET", | ||
| headers: { | ||
| ...headers, | ||
| Accept: "text/event-stream", | ||
| }, | ||
| signal: ctrl.signal, |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
Does @microsoft/fetch-event-source support sending an Authorization header for SSE requests, and is embedding tokens in the URL discouraged?
💡 Result:
Yes, @microsoft/fetch-event-source fully supports sending Authorization headers for Server-Sent Events (SSE) requests [1][2]. Unlike the native browser EventSource API, which prohibits custom headers [1][3], this library leverages the Fetch API, allowing you to pass a standard headers object to your SSE request [4][5]. You can even pass headers as a function to support dynamic, short-lived tokens (e.g., refreshing a Bearer token during retries) [6]. Embedding authentication tokens directly in the URL (e.g., query parameters) is strongly discouraged [3][7][8]. This practice is considered a significant security risk for several reasons: - Leakage in Logs: Authentication tokens will appear in plain text within server access logs (such as those from Nginx, Apache, or cloud load balancers) [3][7][9]. - Browser History and Referrer Headers: Tokens can be persisted in browser history or inadvertently leaked to external sites via Referer headers [3][7][9]. - Caching Risks: Proxies and other intermediary network components may cache the request URL, potentially exposing the token to unauthorized parties [7]. Given these risks, using headers—facilitated by libraries like @microsoft/fetch-event-source or by using the native fetch API with ReadableStream—is the industry-standard approach for authenticated SSE [2][7][8].
Citations:
- 1: https://github.com/azure/fetch-event-source
- 2: https://safeguard.sh/resources/blog/fetch-event-source-sse-guide
- 3: https://www.server-sent-events.com/sse-protocol-fundamentals-architecture/security-headers-for-event-streams/authenticating-sse-streams-with-tokens-and-cookies/
- 4: https://www.npmjs.com/package/@microsoft/fetch-event-source
- 5: https://github.com/Azure/fetch-event-source
- 6: feat(headers): allow headers to be defined as a function to return dynamic values Azure/fetch-event-source#67
- 7: https://openillumi.com/en/en-eventsource-auth-header-solution/
- 8: https://medium.com/@ravindifernando3/why-authenticated-sse-streaming-is-harder-than-it-looks-717f72f512dc
- 9: https://bedirtapkan.com/posts/blog_posts/agents_sdk_template/03-authentication-and-security/
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'Candidate files:\n'
fd -a 'notification\.ts$|package\.json$' . | sed 's#^\./##'
printf '\nRelevant notification.ts excerpt:\n'
if [ -f jobdri/src/lib/api/notification.ts ]; then
nl -ba jobdri/src/lib/api/notification.ts | sed -n '1,120p'
fi
printf '\nAuthorization headers across repo (focused list):\\n'
rg -n "Authorization|accessToken|fetchEventSource|EventSource" jobdri -S || true
printf '\nImports/dependencies for fetch-event-source:\n'
rg -n "`@microsoft/fetch-event-source`|fetch-event-source|fetchEventSource" . -S || trueRepository: JobDri-Developer/FrontEnd
Length of output: 403
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'Relevant notification.ts excerpt:\n'
if [ -f jobdri/src/lib/api/notification.ts ]; then
awk '{printf "%6d\t%s\n", NR, $0}' jobdri/src/lib/api/notification.ts | sed -n '1,120p'
fi
printf '\nAuthorization headers across repo:\n'
grep -RInE "Authorization|accessToken|fetchEventSource|EventSource" jobdri || true
printf '\nImports/dependencies for fetch-event-source:\n'
grep -RInE "`@microsoft/fetch-event-source`|fetch-event-source|fetchEventSource" . || true
printf '\nPackage dependency excerpt:\n'
grep -A40 '"dependencies"' jobdri/package.json | sed -n '1,60p'Repository: JobDri-Developer/FrontEnd
Length of output: 8385
Stop duplicating the access token in the SSE URL.
fetchEventSource sends the Authorization header, so ?accessToken=${tokenOnly} is redundant and exposes the bearer token in server/proxy logs and history. Use the header-only URL unless the backend/gateway explicitly requires the query token.
🔒 Suggested fix
- const tokenOnly = headers.Authorization.replace("Bearer ", "");
-
fetchEventSource(
- `${API_BASE_URL}/api/notifications/stream?accessToken=${tokenOnly}`,
+ `${API_BASE_URL}/api/notifications/stream`,
{
method: "GET",
headers: {
...headers,
Accept: "text/event-stream",
},📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export function subscribeToNotificationStream( | |
| onMessage: (data: ApiNotificationItem) => void, | |
| onError?: (error: unknown) => void, | |
| ) { | |
| const headers = getAuthHeaders(); | |
| const ctrl = new AbortController(); | |
| if (!headers.Authorization) { | |
| console.warn("로그인 토큰이 없어 실시간 알림을 연결하지 않습니다."); | |
| return () => ctrl.abort(); | |
| } | |
| const tokenOnly = headers.Authorization.replace("Bearer ", ""); | |
| fetchEventSource( | |
| `${API_BASE_URL}/api/notifications/stream?accessToken=${tokenOnly}`, | |
| { | |
| method: "GET", | |
| headers: { | |
| ...headers, | |
| Accept: "text/event-stream", | |
| }, | |
| signal: ctrl.signal, | |
| export function subscribeToNotificationStream( | |
| onMessage: (data: ApiNotificationItem) => void, | |
| onError?: (error: unknown) => void, | |
| ) { | |
| const headers = getAuthHeaders(); | |
| const ctrl = new AbortController(); | |
| if (!headers.Authorization) { | |
| console.warn("로그인 토큰이 없어 실시간 알림을 연결하지 않습니다."); | |
| return () => ctrl.abort(); | |
| } | |
| fetchEventSource( | |
| `${API_BASE_URL}/api/notifications/stream`, | |
| { | |
| method: "GET", | |
| headers: { | |
| ...headers, | |
| Accept: "text/event-stream", | |
| }, | |
| signal: ctrl.signal, |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@jobdri/src/lib/api/notification.ts` around lines 60 - 82, Update
subscribeToNotificationStream so fetchEventSource uses the notifications stream
endpoint without the accessToken query parameter; retain the Authorization
header from headers and remove the now-unused tokenOnly extraction.
2 participants
🔗 관련 이슈
#88
📝 개요
⌨️ 작업 상세 내용
1. SSE(Server-Sent Events) 기반 실시간 알림 연동
2. 알림 클릭 시 라우팅 버그 수정 및 예외 처리
mockApplyId추출 로직 수정: 알림 클릭 시 엉뚱한 라우팅이 발생하던 원인(targetId혼용)을 제거하고, 오직payload.mockApplyId만을 엄격하게 추출하여 사용하도록mapApiToLnbItem매핑 함수를 개선했습니다.apiType에 따라 정상적으로 결과 페이지 및 공고 페이지로 이동하도록switch-case문을 정교하게 구성했습니다.mockApplyId가 없는 예외적인 알림(일반 공지 등)의 경우, 에러 없이 읽음 처리만 수행하고 앱이 중단되지 않도록 방어 로직을 추가했습니다.3. '모두 읽음' 기능의 낙관적 업데이트(Optimistic Update) 적용
onMarkAllRead함수를Lnb➔LnbDefaultFooter➔LnbNotificationButton➔LnbNotificationPanel하위 뎁스까지 정상적으로 연결했습니다.4. 미확인 알림 최상단 정렬 (Sorting)
read: false)이 읽은 알림(read: true)보다 무조건 위로 올라오도록.sort()로직을 추가하여 사용자의 확인 편의성을 높였습니다.5. 오래된 알림 자동 숨김 처리 (Filtering)
read: true)' 상태이면서 동시에 '읽은 시간(readAt)' 기준으로 7일(168시간)이 지난 알림은 목록에서 노출되지 않도록filter로직을 구현했습니다.Date.now()와 같은 Impure Function을 렌더링 중에 직접 호출하여 발생하는 Linter 에러 및 하이드레이션 이슈를 방지하기 위해,useState,useEffect(setTimeout),useMemo를 조합하여 클라이언트 사이드에서 안전하게 시간을 계산하도록 처리했습니다.💡 코드 설명 및 참고사항
📸 스크린샷 (UI 변경 시)
🔍 리뷰 요구사항 (Reviewers)
Summary by CodeRabbit
New Features
Bug Fixes